-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cache hit percentage to stats #2211
Conversation
Hi @Xuanwo, Is it OK to you? |
src/server.rs
Outdated
@@ -1510,6 +1510,17 @@ impl Default for ServerStats { | |||
} | |||
} | |||
|
|||
macro_rules! set_percentage_stat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a macro and not a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picked up code from #614 and experienced tunnel vision.
Function is probably a better choice here indeed.
Could you please add a test to verify the field is present? Thanks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2211 +/- ##
==========================================
+ Coverage 30.91% 40.88% +9.97%
==========================================
Files 53 55 +2
Lines 20112 20673 +561
Branches 9755 9794 +39
==========================================
+ Hits 6217 8453 +2236
- Misses 7922 8091 +169
+ Partials 5973 4129 -1844 ☔ View full report in Codecov by Sentry. |
|
||
let output = writer.get_output(); | ||
|
||
assert!(output.contains("Cache hits rate -")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting case where PerLanguageCount::all()
uses counts.values().sum()
and not self.adv_counts.values().sum()
in case of printing advanced stats.
In practice should never happen because PerLanguageCount::increment()
is used to set the counts.
@@ -1700,7 +1780,7 @@ impl ServerInfo { | |||
|
|||
/// Print info to stdout in a human-readable format. | |||
pub fn print(&self, advanced: bool) { | |||
let (name_width, stat_width) = self.stats.print(advanced); | |||
let (name_width, stat_width) = self.stats.print(&mut StdoutServerStatsWriter, advanced); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intrusive way to test the code. If anyone has an idea on how to test it in a better way it would be helpful to know.
I was thinking about writing similar test like in sccache_cargo.rs
(using Command::new()
and capturing the output), but I was hesitant because the test would not be fast.
I was thinking about refactoring this code to free functions and testing the free functions but then it would potentially not cover the whole use case.
Hi @Saruniks Is it ready to be merged? |
Hello @AJIOB Ready from my side. Unless perhaps someone has comments on the code and tests. |
Hi @sylvestre & @Xuanwo |
i was hoping to have also integration tests like in https://github.com/mozilla/sccache/blob/main/.github/workflows/integration-tests.yml |
and sorry for the latency |
Ok. I'll add the integration test |
many thanks :) |
c50cc12
to
dd35cfa
Compare
Hello, a) I added integration tests. Not to the GitHub Actions workflow like mentioned above, but "Rust" ones (see tests/cache_hit_rate.rs). I could add extra jobs to the GitHub Actions workflow, no problems. But I thought that what I developed could be sufficient as no integration with external tool needs to be tested. Let me know if you think otherwise. b) Regarding the json stats: The problem is that json stats is just serialized structs. When "StatsFormat::Text" stats are achieved by using "complex" print method. match fmt {
StatsFormat::Text => stats.print(advanced),
StatsFormat::Json => serde_json::to_writer(&mut io::stdout(), &stats)?,
} I am still not sure what would be the best way to implement cache hit rate for json stats. Maybe custom serialize implementation would work (and not derive trait like currently), but then it could be considered magic-like. But then calculating cache hit rate on every stats change could be considered too wasteful (or considered as calculated value anti-pattern). So not sure how to proceed with json stats. |
tests/helpers/mod.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new code in this file, only moved things around for deduplication
don't bother for the json output |
I guess you saw that some tests are failing |
I fixed the issue with failing tests. Checks should pass now. |
Closes #2161
Inspiration from #614
Feedback is welcome.
No compile requests:
Zero cache hits:
Some cache hits:
sccache --show-adv-stats
: